The Static Analysis tool Coverity added that variable |aContext| can cause a null pointer dereference in the following context: >> { >> mozilla::MutexAutoLock lock(mForcedValidEntriesLock); >> >> for (auto iter = mForcedValidEntries.Iter(); !iter.Done(); iter.Next()) { >> bool matches; >> DebugOnly<nsresult> rv = CacheFileUtils::KeyMatchesLoadContextInfo( >> iter.Key(), aContext, &matches); >> MOZ_ASSERT(NS_SUCCEEDED(rv)); >> >> if (matches) { >> iter.Remove(); >> } >> } >> } This checker was triggered by coverity because of an earlier null check on |aContext| : >> if (aContext && !aContext->IsPrivate()) { >> LOG((" dooming disk entries")); >> CacheFileIOManager::EvictByContext(aContext, aPinned); >> }

I think this is more prone to happen since DoomStorageEntries can get called with |aContext| being explicitly nullptr: >> nsTArray<nsCString> keys; >> for (auto iter = sGlobalEntryTables->Iter(); !iter.Done(); iter.Next()) { >> const nsACString& key = iter.Key(); >> nsCOMPtr<nsILoadContextInfo> info = CacheFileUtils::ParseKey(key); >> if (info && info->IsPrivate()) { >> keys.AppendElement(key); >> } >> } >> >> for (uint32_t i = 0; i < keys.Length(); ++i) { >> DoomStorageEntries(keys[i], nullptr, true, false, nullptr); >> }

Review commit: https://reviewboard.mozilla.org/r/36631/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36631/

Comment on attachment 8723578 [details] MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mayhemer Good catch! But the patch is not fully correct. aContext may be null. In that case we want to delete everything. So, when aContext is null just delete the whole mForcedValidEntries table. BTW, I don't see any dereference here. We pass it to KeyMatchesLoadContextInfo which is fully capable of null-checking...

Thanks for letting me know i will update the patch. Now regarding the null dereference: in function CacheFileUtils::KeyMatchesLoadContextInfo that takes as 2nd argument |aContext| is transferred as |aInfo| and we have: >>nsresult >>KeyMatchesLoadContextInfo(const nsACString &aKey, nsILoadContextInfo *aInfo, >> bool *_retval) >>{ >> nsCOMPtr<nsILoadContextInfo> info = ParseKey(aKey); >> >> if (!info) { >> return NS_ERROR_FAILURE; >> } >> >> *_retval = info->Equals(aInfo); >> return NS_OK; >>} next |aInfo| is transferred as |aOther|: >> bool Equals(nsILoadContextInfo *aOther) >> { >> return IsPrivate() == aOther->IsPrivate() && >> IsAnonymous() == aOther->IsAnonymous() && >> *OriginAttributesPtr() == *aOther->OriginAttributesPtr(); >> } moving on in IsPrivate: >> bool IsPrivate() >> { >> bool pb; >> GetIsPrivate(&pb); >> return pb; >> } dereference happens on line 3: >> GetIsPrivate(&pb); because GetIsPrivate is virtual so this will be dereferenced in order to resolve the function from vtable.

Comment on attachment 8723578 [details] MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mayhemer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36631/diff/1-2/

(In reply to Andi-Bogdan Postelnicu from comment #4) > Thanks for letting me know i will update the patch. > Now regarding the null dereference: Yes! You are right. I misread the argument name. Thanks.

Comment on attachment 8723578 [details] MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mayhemer https://reviewboard.mozilla.org/r/36631/#review33379 ::: netwerk/cache2/CacheStorageService.cpp:1801 (Diff revision 2) > - > + if (aContext) { leave the blank line after mozilla::MutexAutoLock lock(..); ::: netwerk/cache2/CacheStorageService.cpp:1813 (Diff revision 2) > + else { nit: } else {

Comment on attachment 8723578 [details] MozReview Request: Bug 1251253 - prevent null pointer dereference of |aContext| in CacheStorageService::DoomStorageEntries. r?mayhemer Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36631/diff/2-3/